Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicated test #27091

Merged

Conversation

DaveCTurner
Copy link
Contributor

These two tests are (character-for-character) identical, except for the name, which I don't think is important. Can this be removed?

@DaveCTurner DaveCTurner requested a review from imotov October 24, 2017 08:49
@jasontedor
Copy link
Member

Perhaps the intent was to have two slightly different tests, this one testing a timeout situation? Maybe we can suss out what the intent was and fix this by reworking this test to test for that situation?

@imotov
Copy link
Contributor

imotov commented Oct 24, 2017

@jasontedor that's what my thinking as well, but unfortunately I don't remember what my intent was when I added this test 3+ years ago :) However, after checking the code, I think this is a leftover of the origin design of this feature where the master node was supposed to wait for test files from data nodes to appear in the test folder in the repository to make sure they use the same directory. So, the intent was to timeout if files don't show up in time. I vaguely remember, that during the early development of this feature we flipped this logic around and now data nodes verify that the file written by master is visible to them instead. So, I think there is really no point in having this test anymore, since we don't really timeout.

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jasontedor
Copy link
Member

Thanks @imotov.

@DaveCTurner DaveCTurner merged commit cf2d083 into elastic:master Oct 24, 2017
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 24, 2017
* master:
  Timed runnable should delegate to abstract runnable
  Expose adaptive replica selection stats in /_nodes/stats API
  Remove dangerous `ByteBufStreamInput` methods (elastic#27076)
  Blacklist Gradle 4.2 and above
  Remove duplicated test (elastic#27091)
  Update numbers to reflect 4-byte UTF-8-encoded characters (elastic#27083)
  test: avoid generating duplicate multiple fields (elastic#27080)
  Reduce the default number of cached queries. (elastic#26949)
  removed unused import
  ingest: date processor should not fail if timestamp is specified as json number
@DaveCTurner DaveCTurner deleted the 2017-10-24-remove-duplicate-test branch July 23, 2022 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants